fix(router-core,ssr-client): Catch errors thrown during hydrate #5417
Conversation
WalkthroughAdds two not-found test routes (via-head, via-loader-with-context) for React and Solid, and fixes SSR/client hydration and head-execution behaviors so head errors are caught, head runs are de-duplicated, and BEFORE_LOAD notFound resolution climbs to a parent route with a notFoundComponent. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SSRClient as SSR Client (hydrate)
participant Router
participant Route
participant Head as head()
Note over Client,Head: Old flow — uncaught head error during hydrate
Client->>SSRClient: hydrate(router)
SSRClient->>Router: rebuild matches & contexts
SSRClient->>Route: execute head()
Route-->>SSRClient: throws notFound() (uncaught)
SSRClient-->>Client: error propagates — hydrate fails
Note over Client,Head: New flow — caught and logged
Client->>SSRClient: hydrate(router)
SSRClient->>Router: rebuild matches & contexts
SSRClient->>Route: execute head()
Route-->>SSRClient: throws notFound()
SSRClient->>SSRClient: Promise.all.catch -> log "Error during route context hydration: ..."
SSRClient->>Client: continue — notFoundComponent can render
sequenceDiagram
participant loadMatches
participant Head as head()
participant beforeLoad
participant MatchState
Note over loadMatches,Head: SSR head pre-execution & dedupe
loadMatches->>Head: pre-execute head for match
alt head not executed or loaderData/context changed
Head->>MatchState: set headExecuted=true, store lastHeadLoaderData/lastHeadContext
Head-->>loadMatches: head result merged
else head already executed with same data
Head-->>loadMatches: skip (guard)
end
loadMatches->>beforeLoad: execute beforeLoad
alt beforeLoad throws notFound (BEFORE_LOAD)
loadMatches->>MatchState: ascend to closest parent with notFoundComponent
loadMatches->>MatchState: preload notFoundComponent on server (if available)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
roduyemi
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/router-core/tests/hydrate.test.ts (1)
1-254: LGTM - Comprehensive test coverage.This test suite thoroughly validates the hydrate function's behavior, including the critical error handling during route context hydration (lines 215-254) which confirms the fix for issue #5322.
Consider addressing the import ordering for consistency:
-import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { + createMemoryHistory, + createRootRoute, + createRoute, + createRouter, + notFound, +} from '../../react-router/dist/esm' import { hydrate } from '../src/ssr/ssr-client' import type { TsrSsrGlobal } from '../src/ssr/ssr-client' -import { - createMemoryHistory, - createRootRoute, - createRoute, - createRouter, - notFound, -} from '../../react-router/dist/esm' import type { AnyRouteMatch } from '../src'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
e2e/react-start/basic/src/routeTree.gen.ts(10 hunks)e2e/react-start/basic/src/routes/not-found/index.tsx(1 hunks)e2e/react-start/basic/src/routes/not-found/via-head.tsx(1 hunks)e2e/react-start/basic/tests/not-found.spec.ts(3 hunks)e2e/solid-start/basic/src/routeTree.gen.ts(10 hunks)e2e/solid-start/basic/src/routes/not-found/index.tsx(1 hunks)e2e/solid-start/basic/src/routes/not-found/via-head.tsx(1 hunks)e2e/solid-start/basic/tests/not-found.spec.ts(3 hunks)packages/react-router/tests/store-updates-during-navigation.test.tsx(1 hunks)packages/router-core/src/load-matches.ts(1 hunks)packages/router-core/src/ssr/ssr-client.ts(1 hunks)packages/router-core/tests/hydrate.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript in strict mode with extensive type safety across the codebase
Files:
e2e/react-start/basic/src/routes/not-found/index.tsxpackages/router-core/src/ssr/ssr-client.tspackages/router-core/tests/hydrate.test.tse2e/solid-start/basic/src/routes/not-found/via-head.tsxpackages/router-core/src/load-matches.tse2e/solid-start/basic/tests/not-found.spec.tse2e/react-start/basic/src/routes/not-found/via-head.tsxe2e/solid-start/basic/src/routes/not-found/index.tsxe2e/react-start/basic/src/routeTree.gen.tse2e/solid-start/basic/src/routeTree.gen.tspackages/react-router/tests/store-updates-during-navigation.test.tsxe2e/react-start/basic/tests/not-found.spec.ts
**/src/routes/**
📄 CodeRabbit inference engine (AGENTS.md)
Place file-based routes under src/routes/ directories
Files:
e2e/react-start/basic/src/routes/not-found/index.tsxe2e/solid-start/basic/src/routes/not-found/via-head.tsxe2e/react-start/basic/src/routes/not-found/via-head.tsxe2e/solid-start/basic/src/routes/not-found/index.tsx
e2e/**
📄 CodeRabbit inference engine (AGENTS.md)
Store end-to-end tests under the e2e/ directory
Files:
e2e/react-start/basic/src/routes/not-found/index.tsxe2e/solid-start/basic/src/routes/not-found/via-head.tsxe2e/solid-start/basic/tests/not-found.spec.tse2e/react-start/basic/src/routes/not-found/via-head.tsxe2e/solid-start/basic/src/routes/not-found/index.tsxe2e/react-start/basic/src/routeTree.gen.tse2e/solid-start/basic/src/routeTree.gen.tse2e/react-start/basic/tests/not-found.spec.ts
packages/router-core/**
📄 CodeRabbit inference engine (AGENTS.md)
Keep framework-agnostic core router logic in packages/router-core/
Files:
packages/router-core/src/ssr/ssr-client.tspackages/router-core/tests/hydrate.test.tspackages/router-core/src/load-matches.ts
packages/{react-router,solid-router}/**
📄 CodeRabbit inference engine (AGENTS.md)
Implement React and Solid bindings/components only in packages/react-router/ and packages/solid-router/
Files:
packages/react-router/tests/store-updates-during-navigation.test.tsx
🧠 Learnings (1)
📚 Learning: 2025-10-08T08:11:47.064Z
Learnt from: nlynzaad
PR: TanStack/router#5402
File: packages/router-generator/tests/generator/no-formatted-route-tree/routeTree.nonnested.snapshot.ts:19-21
Timestamp: 2025-10-08T08:11:47.064Z
Learning: Test snapshot files in the router-generator tests directory (e.g., files matching the pattern `packages/router-generator/tests/generator/**/routeTree*.snapshot.ts` or `routeTree*.snapshot.js`) should not be modified or have issues flagged, as they are fixtures used to verify the generator's output and are intentionally preserved as-is.
Applied to files:
packages/router-core/tests/hydrate.test.ts
🧬 Code graph analysis (5)
e2e/react-start/basic/src/routes/not-found/index.tsx (2)
e2e/react-start/basic/src/routes/not-found/via-head.tsx (1)
Route(3-15)e2e/react-start/basic/src/routes/not-found/route.tsx (1)
Route(4-8)
packages/router-core/tests/hydrate.test.ts (1)
packages/router-core/src/ssr/ssr-client.ts (1)
TsrSsrGlobal(17-28)
e2e/solid-start/basic/src/routes/not-found/via-head.tsx (1)
e2e/solid-start/basic/src/routes/not-found/index.tsx (1)
Route(3-41)
e2e/react-start/basic/src/routes/not-found/via-head.tsx (1)
e2e/react-start/basic/src/routes/not-found/index.tsx (1)
Route(3-41)
e2e/solid-start/basic/src/routes/not-found/index.tsx (1)
e2e/solid-start/basic/src/routes/not-found/via-head.tsx (1)
Route(3-15)
🪛 ESLint
packages/router-core/tests/hydrate.test.ts
[error] 1-1: Member 'expect' of the import declaration should be sorted alphabetically.
(sort-imports)
[error] 4-10: ../../react-router/dist/esm import should occur before type import of ../src/ssr/ssr-client
(import/order)
🔇 Additional comments (8)
packages/react-router/tests/store-updates-during-navigation.test.tsx (1)
200-200: LGTM - Optimization from improved error handling.The reduction from 7 to 6 updates reflects an optimization in the not-found handling flow introduced by this PR's BEFORE_LOAD improvements.
packages/router-core/src/ssr/ssr-client.ts (1)
213-215: LGTM - Core fix for hydration error handling.This catch handler correctly prevents errors thrown during
head()execution from causing unhandled promise rejections during hydration. The error is logged for debugging, and by this point, the error has already been assigned to the appropriate match (via the BEFORE_LOAD handling inload-matches.ts), so thenotFoundComponentwill render correctly on the client.e2e/react-start/basic/src/routes/not-found/index.tsx (1)
28-37: LGTM - Consistent navigation link addition.The new
via-headlink follows the established pattern and properly integrates with the test infrastructure.e2e/solid-start/basic/src/routes/not-found/index.tsx (1)
28-37: LGTM - Consistent navigation link addition.The new
via-headlink follows the established pattern for Solid Router and properly integrates with the test infrastructure.e2e/solid-start/basic/tests/not-found.spec.ts (2)
12-12: LGTM - Necessary error whitelisting.This whitelisted error message corresponds to the console.error logging introduced in
packages/router-core/src/ssr/ssr-client.tswhen anotFound()error occurs during route context hydration.
28-28: LGTM - Test coverage expansion.Adding
'head'to the thrower sequences enables comprehensive testing of the new not-found error handling path for errors thrown from thehead()function.Also applies to: 57-57
packages/router-core/src/load-matches.ts (1)
56-66: LGTM - Improved BEFORE_LOAD error handling.The new approach of walking up the route tree to find the nearest parent with a
notFoundComponentis more robust than the previous implementation. The subsequent invariant check (lines 79-82) ensures that anotFoundComponentis available, either from the route tree or viadefaultNotFoundComponent.e2e/solid-start/basic/src/routes/not-found/via-head.tsx (1)
1-23: LGTM - Well-structured test route.This route correctly implements the test scenario for
notFound()errors thrown from thehead()function. The test-ids on both components enable proper E2E test verification.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/router-core/tests/hydrate.test.ts (3)
1-11: Fix import sort/order to satisfy lint rules.Alphabetize vitest specifiers and move the react-router import before internal type imports.
As per static analysis hints, apply:
-import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest' -import { hydrate } from '../src/ssr/ssr-client' -import type { TsrSsrGlobal } from '../src/ssr/ssr-client' -import { - createMemoryHistory, - createRootRoute, - createRoute, - createRouter, - notFound, -} from '../../react-router/dist/esm' -import type { AnyRouteMatch } from '../src' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { + createMemoryHistory, + createRootRoute, + createRoute, + createRouter, + notFound, +} from '../../react-router/dist/esm' +import { hydrate } from '../src/ssr/ssr-client' +import type { TsrSsrGlobal } from '../src/ssr/ssr-client' +import type { AnyRouteMatch } from '../src'Based on static analysis hints
46-47: Align test router with client context (optional).You’re hydrating on “client” paths; set isServer to false to avoid future coupling to server-only code paths.
- mockRouter = createRouter({ routeTree, history, isServer: true }) + mockRouter = createRouter({ routeTree, history, isServer: false })
215-254: Make error assertion resilient and ensure spy cleanup.Use expect.objectContaining for partial match and restore the spy in a finally block.
- await hydrate(mockRouter) - - expect(consoleSpy).toHaveBeenCalledWith('Error during route context hydration:', { 'isNotFound': true }) - - consoleSpy.mockRestore() + try { + await hydrate(mockRouter) + expect(consoleSpy).toHaveBeenCalledWith( + 'Error during route context hydration:', + expect.objectContaining({ isNotFound: true }), + ) + } finally { + consoleSpy.mockRestore() + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
e2e/react-start/basic/src/routeTree.gen.ts(10 hunks)e2e/react-start/basic/src/routes/not-found/index.tsx(1 hunks)e2e/react-start/basic/src/routes/not-found/via-head.tsx(1 hunks)e2e/react-start/basic/tests/not-found.spec.ts(3 hunks)e2e/solid-start/basic/src/routeTree.gen.ts(10 hunks)e2e/solid-start/basic/src/routes/not-found/index.tsx(1 hunks)e2e/solid-start/basic/src/routes/not-found/via-head.tsx(1 hunks)e2e/solid-start/basic/tests/not-found.spec.ts(3 hunks)packages/react-router/tests/store-updates-during-navigation.test.tsx(1 hunks)packages/router-core/src/load-matches.ts(1 hunks)packages/router-core/src/ssr/ssr-client.ts(1 hunks)packages/router-core/tests/hydrate.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript in strict mode with extensive type safety across the codebase
Files:
packages/react-router/tests/store-updates-during-navigation.test.tsxpackages/router-core/src/load-matches.tspackages/router-core/src/ssr/ssr-client.tse2e/solid-start/basic/src/routeTree.gen.tse2e/solid-start/basic/src/routes/not-found/index.tsxe2e/react-start/basic/src/routes/not-found/index.tsxe2e/react-start/basic/src/routes/not-found/via-head.tsxe2e/solid-start/basic/tests/not-found.spec.tse2e/solid-start/basic/src/routes/not-found/via-head.tsxe2e/react-start/basic/tests/not-found.spec.tspackages/router-core/tests/hydrate.test.tse2e/react-start/basic/src/routeTree.gen.ts
packages/{react-router,solid-router}/**
📄 CodeRabbit inference engine (AGENTS.md)
Implement React and Solid bindings/components only in packages/react-router/ and packages/solid-router/
Files:
packages/react-router/tests/store-updates-during-navigation.test.tsx
packages/router-core/**
📄 CodeRabbit inference engine (AGENTS.md)
Keep framework-agnostic core router logic in packages/router-core/
Files:
packages/router-core/src/load-matches.tspackages/router-core/src/ssr/ssr-client.tspackages/router-core/tests/hydrate.test.ts
e2e/**
📄 CodeRabbit inference engine (AGENTS.md)
Store end-to-end tests under the e2e/ directory
Files:
e2e/solid-start/basic/src/routeTree.gen.tse2e/solid-start/basic/src/routes/not-found/index.tsxe2e/react-start/basic/src/routes/not-found/index.tsxe2e/react-start/basic/src/routes/not-found/via-head.tsxe2e/solid-start/basic/tests/not-found.spec.tse2e/solid-start/basic/src/routes/not-found/via-head.tsxe2e/react-start/basic/tests/not-found.spec.tse2e/react-start/basic/src/routeTree.gen.ts
**/src/routes/**
📄 CodeRabbit inference engine (AGENTS.md)
Place file-based routes under src/routes/ directories
Files:
e2e/solid-start/basic/src/routes/not-found/index.tsxe2e/react-start/basic/src/routes/not-found/index.tsxe2e/react-start/basic/src/routes/not-found/via-head.tsxe2e/solid-start/basic/src/routes/not-found/via-head.tsx
🧬 Code graph analysis (5)
e2e/solid-start/basic/src/routes/not-found/index.tsx (1)
e2e/solid-start/basic/src/routes/not-found/via-head.tsx (1)
Route(3-15)
e2e/react-start/basic/src/routes/not-found/index.tsx (2)
e2e/react-start/basic/src/routes/not-found/via-head.tsx (1)
Route(3-15)e2e/react-start/basic/src/routes/not-found/route.tsx (1)
Route(4-8)
e2e/react-start/basic/src/routes/not-found/via-head.tsx (1)
e2e/react-start/basic/src/routes/not-found/index.tsx (1)
Route(3-41)
e2e/solid-start/basic/src/routes/not-found/via-head.tsx (1)
e2e/solid-start/basic/src/routes/not-found/index.tsx (1)
Route(3-41)
packages/router-core/tests/hydrate.test.ts (1)
packages/router-core/src/ssr/ssr-client.ts (1)
TsrSsrGlobal(17-28)
🪛 ESLint
packages/router-core/tests/hydrate.test.ts
[error] 1-1: Member 'expect' of the import declaration should be sorted alphabetically.
(sort-imports)
[error] 4-10: ../../react-router/dist/esm import should occur before type import of ../src/ssr/ssr-client
(import/order)
🔇 Additional comments (17)
packages/react-router/tests/store-updates-during-navigation.test.tsx (1)
200-200: Confirm intentional reduction of store updates
- Change from
expect(updates).toBe(7)to6aligns with the optimized notFound BEFORE_LOAD traversal.- Validate no required state updates are skipped in other navigation scenarios.
- Run this test locally multiple times to ensure the count is stable and catches all expected updates.
e2e/solid-start/basic/src/routes/not-found/via-head.tsx (1)
1-23: LGTM!The route implementation correctly demonstrates throwing
notFound()from theheadfunction. The structure follows TanStack Router conventions with proper test IDs for E2E testing, and the component includes a server-awareness attribute for debugging.e2e/react-start/basic/src/routes/not-found/index.tsx (1)
28-37: LGTM!The new link follows the established pattern for the other navigation links and correctly uses the
preloadsearch parameter.packages/router-core/src/ssr/ssr-client.ts (1)
213-215: Appropriate error handling for hydration failures.The catch handler correctly prevents client-side crashes when errors are thrown during route context hydration (including from
head()functions). This aligns with the existing error handling pattern forrouter.load()on line 232, where errors are logged but don't propagate to crash the application.The
notFoundComponentwill render correctly because_handleNotFoundis invoked before this point in the error flow, setting the appropriate match status.e2e/solid-start/basic/src/routes/not-found/index.tsx (1)
28-37: LGTM!The new link correctly follows Solid conventions (using
classinstead ofclassNameand callingpreload()as a function) and maintains consistency with the existing navigation links.e2e/react-start/basic/tests/not-found.spec.ts (2)
12-12: Appropriate error whitelisting for hydration failures.The whitelisted error message matches the console.error output from the hydration catch handler in
packages/router-core/src/ssr/ssr-client.ts(lines 213-214), preventing test failures from expected error logs.
28-28: Expanded test coverage for head lifecycle.Adding
'head'to the thrower sequences appropriately extends test coverage to verify thatnotFound()thrown from theheadfunction is handled correctly in both navigation and direct-visit scenarios.Also applies to: 58-58
e2e/solid-start/basic/tests/not-found.spec.ts (2)
12-12: Appropriate error whitelisting for hydration failures.Matches the React test configuration, whitelisting the expected hydration error message from the catch handler.
28-28: Expanded test coverage for head lifecycle.The thrower sequences now include
'head', maintaining test parity with the React implementation and ensuring comprehensive coverage of notFound handling across all lifecycle phases.Also applies to: 57-57
e2e/react-start/basic/src/routes/not-found/via-head.tsx (1)
1-23: LGTM!The React route implementation correctly demonstrates throwing
notFound()from theheadfunction and follows React conventions (usingclassName). The structure maintains parity with the Solid implementation.packages/router-core/src/load-matches.ts (1)
56-66: Improved notFound handling for BEFORE_LOAD errors.The change from
consttoletforrouteCursorenables traversal up the parent chain to find a suitable route with anotFoundComponentwhen a BEFORE_LOAD error occurs. This ensures that notFound errors thrown duringbeforeLoadare handled by the nearest ancestor that can render a not-found UI, rather than requiring each route to define its ownnotFoundComponent.The logic correctly:
- Only applies to BEFORE_LOAD errors (line 60)
- Traverses parents while no
notFoundComponentexists (line 61)- Updates the error's
routeIdto the handling route (line 65)- Falls back to the existing invariant check if no suitable parent is found (lines 69-82)
e2e/react-start/basic/src/routeTree.gen.ts (3)
37-37: New route import looks correct.Import alias for NotFoundViaHead matches file path and conventions.
173-177: Route wiring LGTM.Id/path '/via-head' under NotFoundRouteRoute aligns with siblings.
281-282: Type tables and children updated consistently.All registries (fullPath/to/id unions, module declarations, and children interfaces/objects) include the new '/not-found/via-head' route. Looks coherent and generated as expected.
Also applies to: 317-318, 358-359, 401-402, 437-438, 477-478, 688-694, 819-820, 826-827
e2e/solid-start/basic/src/routeTree.gen.ts (3)
34-34: New route import looks correct.Alias and path for NotFoundViaHead align with file layout.
160-164: Route instance wiring LGTM.'/via-head' correctly attached under NotFoundRouteRoute.
253-254: Type unions, module mappings, and children updated consistently.All necessary generated surfaces include '/not-found/via-head'. No gaps spotted.
Also applies to: 286-287, 325-326, 364-365, 397-398, 435-436, 634-640, 744-745, 751-752
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/router-core/src/load-matches.ts (1)
60-60: Consider improving type safety for routerCode check.The type assertion
(err as any).routerCodeis not type-safe. Consider adding proper typing for error objects with router codes.If NotFoundError type can be extended, consider:
interface RouterError extends NotFoundError { routerCode?: string }Then check with proper typing:
- if ((err as any).routerCode === 'BEFORE_LOAD' && routeCursor.parentRoute) { + if ('routerCode' in err && err.routerCode === 'BEFORE_LOAD' && routeCursor.parentRoute) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/router-core/src/load-matches.ts(6 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript in strict mode with extensive type safety across the codebase
Files:
packages/router-core/src/load-matches.ts
packages/router-core/**
📄 CodeRabbit inference engine (AGENTS.md)
Keep framework-agnostic core router logic in packages/router-core/
Files:
packages/router-core/src/load-matches.ts
🔇 Additional comments (2)
packages/router-core/src/load-matches.ts (2)
418-421: LGTM!The async batch correctly awaits the resolve function, which is necessary to handle the asynchronous head execution added in the resolve function.
456-481: LGTM!The async conversion of
updateContextis correct and maintains proper sequencing. Allresolve()calls are properly awaited, ensuring head execution completes before state updates.
| let routeCursor = | ||
| inner.router.routesById[err.routeId ?? ''] ?? inner.router.routeTree | ||
|
|
||
| // For BEFORE_LOAD errors, find a parent route with a notFoundComponent that can handle the error | ||
| if ((err as any).routerCode === 'BEFORE_LOAD' && routeCursor.parentRoute) { | ||
| while (routeCursor.parentRoute && !routeCursor.options.notFoundComponent) { | ||
| routeCursor = routeCursor.parentRoute | ||
| } | ||
| // Update the error to point to the error handling route | ||
| err.routeId = routeCursor.id | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify the NotFound ancestor routing behavior.
The new logic correctly climbs the route tree to find an ancestor with a notFoundComponent when handling BEFORE_LOAD errors. This is a significant change in error handling behavior.
Run the following script to verify this logic is exercised in tests:
🏁 Script executed:
#!/bin/bash
# Description: Find test coverage for BEFORE_LOAD NotFound handling
# Search for tests that exercise NotFound errors during beforeLoad
rg -nP -A5 -B5 'BEFORE_LOAD|beforeLoad.*notFound|notFound.*beforeLoad' --type=ts --type=tsx -g '**/*test*' -g '**/*spec*'
# Search for routes with notFoundComponent to understand the hierarchy
ast-grep --pattern $'notFoundComponent: $_'Length of output: 27
🏁 Script executed:
#!/bin/bash
# Retry search for test coverage of BEFORE_LOAD NotFound handling without type filters
# Search for tests mentioning BEFORE_LOAD or notFound in .ts/.js test files
rg -nP -C3 'BEFORE_LOAD|beforeLoad|notFound' -g '**/*.{test,spec}.{ts,js}'
# Search for definitions of notFoundComponent across codebase
rg -nP 'notFoundComponent' -g '**/*.{ts,js}'Length of output: 64948
Add unit tests for BEFORE_LOAD notFound ancestor routing.
Current tests don’t cover the case where a nested route’s beforeLoad throws notFound() and the logic climbs to a parent route with a notFoundComponent; please add tests to assert that err.routeId is updated to the correct ancestor.
| // if there is no `beforeLoad` option, skip everything, batch update the store, return early | ||
| if (!route.options.beforeLoad) { | ||
| batch(() => { | ||
| batch(async () => { |
There was a problem hiding this comment.
batch does not support async
There was a problem hiding this comment.
I have removed the async callback to batch. May you please help re-review. Thanks.
| } | ||
|
|
||
| try { | ||
| // Execute head functions first during SSR |
There was a problem hiding this comment.
head must be executed AFTER beforeLoad and loaders ran, since it can consume context and loaderData
on main, head executes out of order, which is a bug. we need to serialize execution of head after all loaders ran
There was a problem hiding this comment.
Hey @schiller-manuel, there is a bug in main whereby the head is not executing before the beforeLoad. Scripts etc. contained in the head that apply css should run before throwing an error so that the notFoundComponent displays the correct styles. This PR fixes this issue.
There was a problem hiding this comment.
May you please explain what you mean by this?
on main, head executes out of order, which is a bug. we need to serialize execution of head after all loaders ran
…handling when multiple loaders
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
e2e/react-start/basic/src/routeTree.gen.ts(10 hunks)e2e/react-start/basic/src/routes/not-found/index.tsx(1 hunks)e2e/react-start/basic/src/routes/not-found/via-loaders.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript in strict mode with extensive type safety across the codebase
Files:
e2e/react-start/basic/src/routes/not-found/via-loaders.tsxe2e/react-start/basic/src/routeTree.gen.tse2e/react-start/basic/src/routes/not-found/index.tsx
**/src/routes/**
📄 CodeRabbit inference engine (AGENTS.md)
Place file-based routes under src/routes/ directories
Files:
e2e/react-start/basic/src/routes/not-found/via-loaders.tsxe2e/react-start/basic/src/routes/not-found/index.tsx
e2e/**
📄 CodeRabbit inference engine (AGENTS.md)
Store end-to-end tests under the e2e/ directory
Files:
e2e/react-start/basic/src/routes/not-found/via-loaders.tsxe2e/react-start/basic/src/routeTree.gen.tse2e/react-start/basic/src/routes/not-found/index.tsx
🧬 Code graph analysis (2)
e2e/react-start/basic/src/routes/not-found/via-loaders.tsx (2)
e2e/react-start/basic/src/routes/not-found/index.tsx (1)
Route(3-51)e2e/react-start/basic/src/routes/not-found/via-head.tsx (1)
Route(3-15)
e2e/react-start/basic/src/routes/not-found/index.tsx (2)
e2e/react-start/basic/src/routes/not-found/via-loaders.tsx (1)
Route(3-24)e2e/react-start/basic/src/routes/not-found/via-head.tsx (1)
Route(3-15)
🔇 Additional comments (3)
e2e/react-start/basic/src/routes/not-found/index.tsx (1)
28-47: LGTM! Consistent link structure for new test routes.The new links follow the established pattern and properly wire up the via-head and via-loaders test routes with correct data-testid attributes for e2e testing.
e2e/react-start/basic/src/routes/not-found/via-loaders.tsx (1)
9-14: Clarify the test intent: both loader and head throw notFound().Both the
loader(line 10) andhead(line 13) thrownotFound(). Since the loader executes before head in the route lifecycle, the head function will never be reached if the loader throws. This might be intentional to test error handling precedence, but it's unclear.If the intent is to test head-stage notFound handling, consider removing the
throw notFound()from the loader. If testing loader precedence, the head throw is redundant. Please clarify the test scenario this route is meant to exercise.e2e/react-start/basic/src/routeTree.gen.ts (1)
1-1005: Auto-generated route tree updates look correct.This file is auto-generated by TanStack Router (as noted in lines 7-9). The additions for
NotFoundViaHeadRouteandNotFoundViaLoadersRouteare mechanically correct and consistent with the existing route structure. Any issues with the source routes will be reflected here once those files are corrected.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
e2e/react-start/basic/src/routes/not-found/via-loaders.tsx(1 hunks)e2e/react-start/basic/tests/not-found.spec.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- e2e/react-start/basic/src/routes/not-found/via-loaders.tsx
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript in strict mode with extensive type safety across the codebase
Files:
e2e/react-start/basic/tests/not-found.spec.ts
e2e/**
📄 CodeRabbit inference engine (AGENTS.md)
Store end-to-end tests under the e2e/ directory
Files:
e2e/react-start/basic/tests/not-found.spec.ts
🔇 Additional comments (2)
e2e/react-start/basic/tests/not-found.spec.ts (2)
12-12: LGTM: Appropriate error whitelisting for hydration scenario.The new whitelisted error message correctly handles the expected error output when
notFound()is thrown during client-side hydration, which is the core issue addressed by this PR.
28-28: LGTM: Comprehensive test coverage for notFound() scenarios.The expansion to include
'head'is essential for validating the PR fix, and adding'beforeLoad'and'loaders'ensures comprehensive coverage across all lifecycle hooks wherenotFound()can be thrown during navigation.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/router-core/src/load-matches.ts (1)
892-943: Clarify or eliminate duplicate head execution during SSR.Head is executed twice during SSR: once before
beforeLoad(lines 892-912) and again after all loaders complete (lines 925-943). While the guards inexecuteHeadprevent redundant work, this pattern is confusing and seems to contradict the stated architecture where head should execute after loaders.If the pre-beforeLoad head execution serves a specific purpose (e.g., ensuring CSS is loaded before error rendering), please document this intent with a clear comment explaining why head must execute twice and why it's safe to do so before context/loaderData are available. Otherwise, remove the first execution (lines 892-912) to align with the architectural guidance.
♻️ Duplicate comments (2)
packages/router-core/src/load-matches.ts (2)
53-73: Add unit tests for BEFORE_LOAD notFound ancestor routing.The logic for climbing the route tree to find an ancestor with a
notFoundComponentduring BEFORE_LOAD errors is a significant behavioral change. Tests should verify thaterr.routeIdis correctly updated to the ancestor route and that the notFoundComponent is properly resolved.Based on past review comments.
925-943: Handle redirect and notFound errors from head execution.The catch blocks only log warnings without proper error handling. If
head()throwsnotFound()orredirect(), these should be handled appropriately rather than being swallowed. This is especially important since the PR objective is to properly catch and handle errors thrown during hydrate, including from head functions.Apply this diff to handle errors properly:
try { const headResult = executeHead(inner, matchId, route) if (headResult) { const head = await headResult inner.updateMatch(matchId, (prev) => ({ ...prev, ...head, })) } } catch (err) { + if (isRedirect(err) || isNotFound(err)) { + handleRedirectAndNotFound(inner, inner.router.getMatch(matchId), err) + } - console.warn('Error executing head during SSR:', err) + console.warn('Error executing head during SSR:', err) }Based on past review comments.
🧹 Nitpick comments (2)
packages/router-core/src/Matches.ts (1)
151-152: Consider adding documentation for these internal fields.The new
headExecutedandlastHeadLoaderDatafields lack the/** @internal */JSDoc tags that other internal fields in this section have (e.g.,beforeLoadPromise,loaderPromise). Adding these tags and brief descriptions would improve consistency and clarify their purpose, especiallylastHeadLoaderData, which could benefit from explaining what it represents (e.g., whether it's a snapshot of loader data when head was executed).Apply this diff to add documentation:
dehydrated?: boolean + /** @internal */ headExecuted?: boolean + /** @internal */ lastHeadLoaderData?: TLoaderData }e2e/solid-start/basic/src/routes/not-found/via-loaders.tsx (1)
19-19: Fix data-server attribute to use a boolean or string "true"/"false".The expression
data-server={typeof window}evaluates to the string"undefined"on the server or"object"on the client. For test assertions, a boolean value or explicit string"true"/"false"would be clearer.Apply this diff:
- <div data-testid="via-loaders-notFound-component" data-server={typeof window}> + <div data-testid="via-loaders-notFound-component" data-server={typeof window === 'undefined' ? 'true' : 'false'}>The same issue exists on line 28 in
RouteComponent.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
e2e/react-start/basic/tests/not-found.spec.ts(3 hunks)e2e/react-start/basic/vite.config.ts(1 hunks)e2e/solid-start/basic/src/routeTree.gen.ts(10 hunks)e2e/solid-start/basic/src/routes/not-found/index.tsx(1 hunks)e2e/solid-start/basic/src/routes/not-found/via-loaders.tsx(1 hunks)e2e/solid-start/basic/tests/not-found.spec.ts(3 hunks)packages/router-core/src/Matches.ts(1 hunks)packages/router-core/src/load-matches.ts(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- e2e/react-start/basic/tests/not-found.spec.ts
- e2e/solid-start/basic/src/routeTree.gen.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript in strict mode with extensive type safety across the codebase
Files:
e2e/solid-start/basic/src/routes/not-found/via-loaders.tsxpackages/router-core/src/load-matches.tse2e/react-start/basic/vite.config.tspackages/router-core/src/Matches.tse2e/solid-start/basic/tests/not-found.spec.tse2e/solid-start/basic/src/routes/not-found/index.tsx
**/src/routes/**
📄 CodeRabbit inference engine (AGENTS.md)
Place file-based routes under src/routes/ directories
Files:
e2e/solid-start/basic/src/routes/not-found/via-loaders.tsxe2e/solid-start/basic/src/routes/not-found/index.tsx
e2e/**
📄 CodeRabbit inference engine (AGENTS.md)
Store end-to-end tests under the e2e/ directory
Files:
e2e/solid-start/basic/src/routes/not-found/via-loaders.tsxe2e/react-start/basic/vite.config.tse2e/solid-start/basic/tests/not-found.spec.tse2e/solid-start/basic/src/routes/not-found/index.tsx
packages/router-core/**
📄 CodeRabbit inference engine (AGENTS.md)
Keep framework-agnostic core router logic in packages/router-core/
Files:
packages/router-core/src/load-matches.tspackages/router-core/src/Matches.ts
🧬 Code graph analysis (3)
e2e/solid-start/basic/src/routes/not-found/via-loaders.tsx (1)
e2e/solid-start/basic/src/routes/not-found/index.tsx (1)
Route(3-51)
packages/router-core/src/load-matches.ts (1)
packages/router-core/src/utils.ts (1)
isPromise(471-479)
e2e/solid-start/basic/src/routes/not-found/index.tsx (2)
e2e/solid-start/basic/src/routes/not-found/via-loaders.tsx (1)
Route(3-24)e2e/solid-start/basic/src/routes/not-found/via-head.tsx (1)
Route(3-15)
🔇 Additional comments (6)
e2e/react-start/basic/vite.config.ts (1)
15-28: LGTM! Prerender exclusions align with the PR objectives.The addition of
/not-found/via-headand/not-found/via-loadersto the prerender filter correctly excludes these routes from static generation. This is appropriate since these routes are designed to test error handling during client-side hydration, which requires runtime execution rather than prerendering.packages/router-core/src/load-matches.ts (1)
537-566: Optimization guards look reasonable.The guards to skip redundant head execution are appropriate. The referential equality check on
loaderDataassumes stable references, which appears to be the case in this codebase.e2e/solid-start/basic/src/routes/not-found/index.tsx (1)
28-47: LGTM! Consistent navigation link additions.The new navigation links for
via-headandvia-loadersfollow the same pattern as existing links, with proper preload support and test IDs that align with the expanded test coverage.e2e/solid-start/basic/tests/not-found.spec.ts (2)
12-12: Good addition to whitelist expected hydration error.This error message aligns with the PR objective of catching errors thrown during client-side hydrate. The whitelisted error ensures tests pass when
notFound()is thrown during hydration.
28-28: Excellent expansion of test coverage across lifecycle hooks.Expanding the thrower matrices to include
'beforeLoad','head','loader', and'loaders'ensures thatnotFound()handling works correctly across all route lifecycle hooks, both during navigation and direct visits. This directly addresses the PR objective of catching head function errors during hydrate.Also applies to: 57-57
e2e/solid-start/basic/src/routes/not-found/via-loaders.tsx (1)
16-23: Verify that beforeLoad context is available when loader throws notFound().The
notFoundComponentaccessescontext().foolfrom thebeforeLoadreturn value. Ensure thatbeforeLoadcompletes beforeloaderthrows, so the context is available to thenotFoundComponent.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
packages/router-core/src/load-matches.ts (3)
56-73: Add unit tests for BEFORE_LOAD notFound ancestor routing.The logic correctly climbs the route tree to find an ancestor with a
notFoundComponentwhen handling BEFORE_LOAD errors. However, as noted in previous reviews, test coverage is still missing for this scenario.Please add tests to verify:
- A nested route's
beforeLoadthrowsnotFound()- The logic climbs to the correct parent route with a
notFoundComponenterr.routeIdis updated to the ancestor route ID- The ancestor's
notFoundComponentis properly preloaded on the serverBased on past review comments.
892-912: Executing head before beforeLoad contradicts architectural requirements.As noted in previous reviews and confirmed by the maintainer, "head must be executed AFTER beforeLoad and loaders ran, since it can consume context and loaderData." Executing head before
beforeLoad(lines 892-912) violates this requirement because:
- The
headfunction won't have access tocontextpopulated by parentbeforeLoadfunctions- The
headfunction won't have access toloaderDatafrom loaders- The
assetContextpassed toheadat line 547-552 includesmatch.loaderData, which will be undefined at this stageAdditionally, the catch block only logs warnings without properly handling
redirect()ornotFound()errors that may be thrown from head execution.While the concern about CSS loading for
notFoundComponentstyling is valid, executingheadprematurely breaks the architectural contract. Consider alternative solutions:
- Recommended approach: Ensure
notFoundComponenthas inline critical CSS or default styling that doesn't depend on head execution- Alternative: If head execution is required before error handling, add special handling that executes a limited subset of head (e.g., only critical CSS) without requiring full context/loaderData
If pre-beforeLoad head execution is kept, at minimum handle redirect/notFound errors:
} catch (err) { // Don't let head execution errors break beforeLoad execution - console.warn('Error executing head before beforeLoad:', err) + if (isRedirect(err) || isNotFound(err)) { + throw err + } + console.warn('Error executing head before beforeLoad:', err) }Based on past review comments.
925-943: Handle redirect and notFound errors from head execution.The catch block only logs warnings without proper handling. If
head()throwsnotFound()orredirect(), these should be handled appropriately rather than being swallowed, as they represent intentional control flow signals.Apply this diff to properly handle errors:
} catch (err) { - console.warn('Error executing head during SSR:', err) + if (isRedirect(err) || isNotFound(err)) { + throw err + } + console.warn('Error executing head during SSR:', err) }Based on past review comments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/router-core/src/load-matches.ts(6 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript in strict mode with extensive type safety across the codebase
Files:
packages/router-core/src/load-matches.ts
packages/router-core/**
📄 CodeRabbit inference engine (AGENTS.md)
Keep framework-agnostic core router logic in packages/router-core/
Files:
packages/router-core/src/load-matches.ts
🧬 Code graph analysis (1)
packages/router-core/src/load-matches.ts (1)
packages/router-core/src/utils.ts (1)
isPromise(471-479)
🔇 Additional comments (1)
packages/router-core/src/load-matches.ts (1)
892-943: Request clarification on head execution architecture.The current implementation executes
headtwice during SSR (before beforeLoad at lines 892-912, and after loaders at lines 925-943), which creates a complex flow. The guard at lines 537-541 attempts to prevent duplicate work, but as noted in another comment, it has flaws.There appears to be an unresolved architectural decision between:
- Maintainer's requirement: Head must execute after beforeLoad/loaders to access context and loaderData
- PR author's concern: CSS from head needs to load before notFoundComponent displays
Please clarify the intended architecture. Questions to address:
- Should head execute before or after beforeLoad? Or both?
- If both, what subset of head functionality should execute early vs. late?
- How should the guard logic account for context changes from parent beforeLoad?
- What is the expected behavior when head throws redirect/notFound before beforeLoad?
Based on past review comments.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
e2e/solid-start/basic/src/routes/not-found/via-loader-with-context.tsx (1)
16-16: Clarify test attribute for server/client distinction.The
data-server={typeof window}attribute will output the string"undefined"on the server and"object"on the client. While this may work for testing, consider using a more explicit boolean or string literal for clearer test assertions:data-server={typeof window === 'undefined' ? 'true' : 'false'}Or simply:
data-is-server={typeof window === 'undefined'}e2e/solid-start/basic/tests/not-found.spec.ts (1)
27-31: Good expansion of test coverage.The test matrix now covers the additional thrower scenarios (
headandloader-with-context) that the PR fix addresses. ThebeforeLoadTODO is acceptable as a known limitation, but consider tracking it in an issue to ensure it's addressed in a follow-up.Would you like me to open an issue to track the
beforeLoadtest coverage?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
e2e/react-start/basic/src/routeTree.gen.ts(10 hunks)e2e/react-start/basic/src/routes/not-found/index.tsx(1 hunks)e2e/react-start/basic/src/routes/not-found/via-loader-with-context.tsx(1 hunks)e2e/react-start/basic/tests/not-found.spec.ts(3 hunks)e2e/react-start/basic/vite.config.ts(1 hunks)e2e/solid-start/basic/src/routeTree.gen.ts(10 hunks)e2e/solid-start/basic/src/routes/not-found/index.tsx(1 hunks)e2e/solid-start/basic/src/routes/not-found/via-loader-with-context.tsx(1 hunks)e2e/solid-start/basic/tests/not-found.spec.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- e2e/react-start/basic/vite.config.ts
- e2e/react-start/basic/tests/not-found.spec.ts
- e2e/solid-start/basic/src/routes/not-found/index.tsx
- e2e/react-start/basic/src/routes/not-found/index.tsx
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript in strict mode with extensive type safety across the codebase
Files:
e2e/react-start/basic/src/routes/not-found/via-loader-with-context.tsxe2e/solid-start/basic/tests/not-found.spec.tse2e/solid-start/basic/src/routes/not-found/via-loader-with-context.tsxe2e/solid-start/basic/src/routeTree.gen.tse2e/react-start/basic/src/routeTree.gen.ts
**/src/routes/**
📄 CodeRabbit inference engine (AGENTS.md)
Place file-based routes under src/routes/ directories
Files:
e2e/react-start/basic/src/routes/not-found/via-loader-with-context.tsxe2e/solid-start/basic/src/routes/not-found/via-loader-with-context.tsx
e2e/**
📄 CodeRabbit inference engine (AGENTS.md)
Store end-to-end tests under the e2e/ directory
Files:
e2e/react-start/basic/src/routes/not-found/via-loader-with-context.tsxe2e/solid-start/basic/tests/not-found.spec.tse2e/solid-start/basic/src/routes/not-found/via-loader-with-context.tsxe2e/solid-start/basic/src/routeTree.gen.tse2e/react-start/basic/src/routeTree.gen.ts
🧬 Code graph analysis (2)
e2e/react-start/basic/src/routes/not-found/via-loader-with-context.tsx (2)
e2e/react-start/basic/src/routes/not-found/index.tsx (1)
Route(3-51)e2e/solid-start/basic/src/routes/not-found/via-loader-with-context.tsx (1)
Route(3-21)
e2e/solid-start/basic/src/routes/not-found/via-loader-with-context.tsx (1)
e2e/solid-start/basic/src/routes/not-found/index.tsx (1)
Route(3-51)
🔇 Additional comments (4)
e2e/solid-start/basic/src/routeTree.gen.ts (1)
34-34: LGTM! Auto-generated route tree correctly integrates new test routes.The route tree generator has properly added the two new not-found test routes (
via-headandvia-loader-with-context) with:
- Correct import statements
- Proper route definitions with appropriate IDs and paths
- Complete type mappings across all relevant interfaces
- Correct parent-child relationships under NotFoundRouteRoute
All additions follow the established patterns and are consistent with the existing route structure.
Also applies to: 36-36, 163-168, 174-178, 268-270, 303-305, 344-346, 385-387, 420-422, 460-462, 661-667, 675-681, 785-787, 793-795
e2e/react-start/basic/src/routes/not-found/via-loader-with-context.tsx (1)
1-29: LGTM! Well-structured test route for notFound with context.This test route correctly validates that context set in
beforeLoadremains accessible tonotFoundComponentwhen theloaderthrowsnotFound(). The implementation uses appropriate React patterns (direct property access on context, not function calls), and the unique data-testid values align with testing requirements for the hydration error-handling fix.e2e/react-start/basic/src/routeTree.gen.ts (1)
36-851: Auto-generated route tree correctly registers new test routes.This file is auto-generated by TanStack Router (per lines 7-9), and the additions properly register both
NotFoundViaLoaderWithContextRouteandNotFoundViaHeadRoute. The generated code follows consistent patterns across imports, route definitions, type mappings, module augmentation, and parent-child relationships.e2e/solid-start/basic/tests/not-found.spec.ts (1)
10-14: LGTM! Whitelisted error aligns with the hydration fix.The new whitelisted error message correctly reflects the expected behavior after the PR fix, which catches and logs errors during route context hydration instead of letting them crash the application.
| const directVisitTestMatrix = combinate({ | ||
| // TODO beforeLoad! | ||
|
|
||
| thrower: [/* 'beforeLoad',*/ 'loader'] as const, | ||
| thrower: [/* 'beforeLoad',*/ 'head', 'loader', 'loader-with-context'] as const, | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
Consistent test matrix expansion for direct visits.
The direct visit test matrix correctly mirrors the navigation test matrix expansion, ensuring comprehensive coverage of both navigation paths and direct URL visits.
The relevant imports reference e2e/react-start/basic/tests/not-found.spec.ts. Please verify that the parallel React test file has also been updated with the same changes (whitelisted error and expanded thrower arrays).
🏁 Script executed:
#!/bin/bash
# Description: Verify that the React parallel test file has matching updates
# Check if the React test file exists and contains the new throwers
if [ -f "e2e/react-start/basic/tests/not-found.spec.ts" ]; then
echo "=== Checking React test file for matching changes ==="
# Check for whitelisted hydration error
echo -e "\n1. Whitelisted error message:"
rg -n "Error during route context hydration" e2e/react-start/basic/tests/not-found.spec.ts
# Check for expanded thrower arrays
echo -e "\n2. Thrower arrays:"
rg -n "thrower:.*head.*loader.*loader-with-context" e2e/react-start/basic/tests/not-found.spec.ts
else
echo "React test file not found - may need to be created or updated"
fiLength of output: 339
Thrower array inconsistency between test files requires resolution.
The React test file includes beforeLoad in both thrower arrays (lines 28, 58), but the Solid Start file explicitly excludes it with a TODO comment (line 59). This inconsistency must be resolved:
- Either enable
beforeLoadin Solid Start to match React, or - Document why the frameworks intentionally differ in thrower coverage
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/router-core/src/load-matches.ts (1)
832-842: Uncaught head errors when loader doesn’t run (hydrate path).When the loader is skipped and you only execute head, errors from head (e.g., notFound/redirect) bubble up to loadMatches’ outer catch, which doesn’t call handleRedirectAndNotFound, so the match never enters notFound/redirected state. This reproduces the original bug on client hydrate.
Wrap this block with a try/catch and route through handleRedirectAndNotFound.
- const headResult = executeHead(inner, matchId, route) - if (headResult) { - const head = await headResult - inner.updateMatch(matchId, (prev) => ({ - ...prev, - ...head, - })) - } + try { + const headResult = executeHead(inner, matchId, route) + if (headResult) { + const head = await headResult + inner.updateMatch(matchId, (prev) => ({ + ...prev, + ...head, + })) + } + } catch (err) { + handleRedirectAndNotFound( + inner, + inner.router.getMatch(matchId), + err, + ) + }
♻️ Duplicate comments (2)
packages/router-core/src/load-matches.ts (2)
893-911: Don’t swallow redirect/notFound from SSR pre-beforeLoad head.The catch only warns. Redirect/notFound thrown here should be handled via handleRedirectAndNotFound to update state and propagate as needed, not suppressed.
- } catch (err) { - // Don't let head execution errors break beforeLoad execution - console.warn('Error executing head before beforeLoad:', err) - } + } catch (err) { + if (isRedirect(err) || isNotFound(err)) { + handleRedirectAndNotFound( + inner, + inner.router.getMatch(matchId), + err, + ) + } else { + // Don't let unexpected head errors break beforeLoad execution + console.warn('Error executing head before beforeLoad:', err) + } + }If keeping this early SSR head execution (see maintainer guidance that head should run after beforeLoad/loaders), please document expectations and ensure head defensively handles missing context/loaderData. Based on past review comments.
923-941: Also handle redirect/notFound in SSR post-load head loop.Same issue as above: the catch only warns; special errors should go through handleRedirectAndNotFound.
- } catch (err) { - console.warn('Error executing head during SSR:', err) - } + } catch (err) { + if (isRedirect(err) || isNotFound(err)) { + handleRedirectAndNotFound( + inner, + inner.router.getMatch(matchId), + err, + ) + } else { + console.warn('Error executing head during SSR:', err) + } + }
🧹 Nitpick comments (1)
packages/router-core/src/load-matches.ts (1)
56-73: BEFORE_LOAD notFound ancestor climb: good, but SSR preload misses default fallback.The climb and routeId reassignment look correct. However, SSR preloading happens before you inject a defaultNotFoundComponent (Lines 75-83), so when the component is inherited from router.options.defaultNotFoundComponent, it won’t be preloaded.
Recommend moving or duplicating the preload after ensuring the final notFoundComponent.
- if (inner.router.isServer) { - const loadPromise = loadRouteChunk(routeCursor) - if (loadPromise) { - loadPromise.then(() => (routeCursor.options.notFoundComponent)?.preload?.()) - } - } + // defer SSR preload until after the notFoundComponent is finalized below…and after Lines 85-90 (post-invariant), add:
+ // Preload the finalized notFoundComponent on the server + if (inner.router.isServer) { + const p = loadRouteChunk(routeCursor) + if (p) { + p.then(() => + (routeCursor.options.notFoundComponent as any)?.preload?.(), + ) + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/router-core/src/load-matches.ts(6 hunks)packages/solid-router/tests/store-updates-during-navigation.test.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript in strict mode with extensive type safety across the codebase
Files:
packages/solid-router/tests/store-updates-during-navigation.test.tsxpackages/router-core/src/load-matches.ts
packages/{react-router,solid-router}/**
📄 CodeRabbit inference engine (AGENTS.md)
Implement React and Solid bindings/components only in packages/react-router/ and packages/solid-router/
Files:
packages/solid-router/tests/store-updates-during-navigation.test.tsx
packages/router-core/**
📄 CodeRabbit inference engine (AGENTS.md)
Keep framework-agnostic core router logic in packages/router-core/
Files:
packages/router-core/src/load-matches.ts
🧠 Learnings (1)
📚 Learning: 2025-10-08T08:11:47.088Z
Learnt from: nlynzaad
PR: TanStack/router#5402
File: packages/router-generator/tests/generator/no-formatted-route-tree/routeTree.nonnested.snapshot.ts:19-21
Timestamp: 2025-10-08T08:11:47.088Z
Learning: Test snapshot files in the router-generator tests directory (e.g., files matching the pattern `packages/router-generator/tests/generator/**/routeTree*.snapshot.ts` or `routeTree*.snapshot.js`) should not be modified or have issues flagged, as they are fixtures used to verify the generator's output and are intentionally preserved as-is.
Applied to files:
packages/solid-router/tests/store-updates-during-navigation.test.tsx
🔇 Additional comments (3)
packages/solid-router/tests/store-updates-during-navigation.test.tsx (1)
203-203: Approve this change—the update count reduction is the expected outcome of the PR's notFound handling improvements.The test shows
expect(updates).toBe(5)on line 203. This reduction aligns perfectly with the PR's goals of improving notFound error handling and de-duplicating operations. The test comment explicitly states that update counts should be minimized, and this change reflects that optimization being realized. The change is correct and requires no further action.packages/router-core/src/load-matches.ts (2)
94-94: No-op change.Whitespace-only; nothing to review.
537-541: Types properly cover all new _nonReactive fields; LGTM.Verification confirms all three fields (
headExecuted,lastHeadLoaderData,lastHeadContext) are defined in the_nonReactiveinterface with appropriate generic types (boolean,TLoaderData,TAllContext) in packages/router-core/src/Matches.ts (lines 151-153), and correctly used in packages/router-core/src/load-matches.ts (lines 539, 565-567). No type safety issues.
|
closing in favour of #5686. @schiller-manuel #5686 only contains changes to catch errors in the |
2 participants
Fixes #5322
Summary by CodeRabbit
New Features
Bug Fixes
Tests